-
Notifications
You must be signed in to change notification settings - Fork 222
Add AwsCredentialFeatures to Credential Providers
#4238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AwsCredentialFeatures to Credential Providers
#4238
Conversation
Also add related integration test for IMDS creds
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
aajtodd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. It seems like we made a lot of stuff public or behind a test-util feature to allow testing via sdk/integration-tests. Part of me wonders if we wouldn't be better off making these unit tests in aws-config OR updating our integration tests to assert a specific UA credential feature
e.g.
make_test!(prefer_environment, feature: AwsCredentialFeature::CredentialsEnvVars)That way we don't have to expose a bunch of test utilities just for this. I'm slightly concerned about having to maintain a bunch of things in test-util because we made them public but open to discussion.
Good point. We should generally place tests in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will circle back for the test part, but looks great. Thank you for making this change 👍
…nfig test-utils Move remaining test into S3 tests since that kind of serves as our catch all for random features
|
We discussed offline and decided that the best course of action, to avoid exposing new |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
ysaito1001
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a CI failure, but the changes look good.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> Merging the feature branch containing work from the below two PRs: * #4238 * #4224 ## Checklist <!--- If a checkbox below is not applicable, then please DELETE it rather than leaving it unchecked --> - [x] For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the `.changelog` directory, specifying "aws-sdk-rust" in the `applies_to` key. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: AWS SDK Rust Bot <[email protected]>
Motivation and Context
Continuing the work in #4224, adding user-agent feature tracking for Credential Providers.
Description
Added
AwsCredentialFeaturesproperties for each implementor ofProvideCredentialsinaws-config.Testing
Added integration tests for each of the supported Credential Providers. This required exposing some previously
pub(crate)utility functions aspubunder atest-utilfeature flag.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.